Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List limits for Limits::downlevel_defaults and downlevel_webgl2_defaults in docs. #3988

Conversation

JustAnotherCodemonkey
Copy link
Contributor

@JustAnotherCodemonkey JustAnotherCodemonkey commented Jul 28, 2023

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description
When looking at the docs for Limits, the actual limits returned by the above default functions are not listed. This PR, among a few other small changes to make things more clear, lists the values for the Limits returned by each of those functions within the docs. This PR makes the following changes:

  1. Lists the values for Limits::downlevel_defaults and Limits::downlevel_webgl2_defaults in the documentation for Limits.
  2. Changes the memory size units used the the docs for Limits to be their binary unit counterparts to avoid confusion (MiB instead of MB as a true megabyte is 1000 KB instead of 1024 even though it is often used to mean the latter, as is done in the docs as they stand).
  3. Refactors the value of max_buffer_size used for both <Limits as Default>::default and downlevel_defaults from 1 << 28 to 256 << 20 for clarity and readability. This follows the pattern of both max_uniform_buffer_binding_size and max_storage_buffer_binding_size which use a similar notation.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements!

personally I'm not a huge fan of duplicating the constant values from the code into the docs (in particular those that aren't dicated by the spec), but I get that it's quite annoying when browsing e.g. docs.rs.

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

To make sure these stay in sync, if you change the list to a doctest, you can assert the documented list is the same as that returned by the function.

@JustAnotherCodemonkey
Copy link
Contributor Author

What do you mean? How do I make a doctest without changing how the docs look in the end result? I would love to do that.

@cwfitzgerald
Copy link
Member

The documentation will end up looking different - it would be in a code block, and we'd need something other than bold to determine which ones have changed.

@JustAnotherCodemonkey
Copy link
Contributor Author

I'll try that. I'll add another commit and see what you think.

@JustAnotherCodemonkey
Copy link
Contributor Author

Ok so I marked those changed from defaults to downlevel defaults are marked with a * and those changed from downlevel defaults in downlevel webgl2 defaults are marked with a + with carryover *'s. I do worry about the accessibility of that it might be hard to distinguish. I've thought of some other symbol combinations but they don't make as much sense. What do you guys think?

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! This is a much better solution!

@Wumpf Wumpf merged commit 5b4c6b8 into gfx-rs:trunk Aug 2, 2023
20 checks passed
@JustAnotherCodemonkey JustAnotherCodemonkey deleted the JustAnotherCodemonkey/add-docs-for-limits-defaults branch August 4, 2023 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants